feat: upload input v3 image preview improvement#263
Conversation
|
Warning Review limit reached
More reviews will be available in 19 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates upload thumbnail previews to create and cache blob URLs for new image uploads, render local URLs synchronously in ChangesBlob URL Preview Caching and Lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 182-194: The current preview assignment uses FIFO shift on
pendingPreviewsRef.current and assumes server-returned value order matches
upload order, causing mis-matches; update the logic to match previews to files
by a stable identifier instead of insertion order: ensure preview entries in
pendingPreviewsRef include the original client filename (e.g.,
preview.originalName or preview.clientFilename) at enqueue time, build a map
from that original name to dataURL, then iterate newFiles (from value) and for
each file try to match by comparing f.filename or f.originalName to the preview
map key (and use a heuristic like contains/endsWith if the server adds
prefixes); assign matched previews into updates and only fall back to shift()
for any remaining unmatched files; update the code paths around
pendingPreviewsRef, newFiles, and setFilePreviews to use this name-based
matching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d9718e3-5485-4d80-a2ed-9b8ff5300c12
📒 Files selected for processing (3)
src/components/inputs/upload-input-v3/dropzone-v3.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/index.js
priscila-moneo
force-pushed
the
feature/upload-input-v3-image-preview-improvement
branch
from
4610604 to
32e967f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/inputs/upload-input-v3/index.js (1)
210-214:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreview mapping is still ambiguous for same-size concurrent uploads.
At Line 212, matching uses only
response.size, so two different files with identical sizes can receive swapped previews. Please map with a stable per-file identifier (client upload id echoed by server, or an explicit correlation token) instead of size-only matching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/index.js` around lines 210 - 214, The current preview-assignment logic in the setUploadingFiles callback uses response.size to match upload entries (response.name/response.size and entry.previewUrl), which causes wrong previews for concurrent same-size files; change the protocol to use a stable per-file identifier (e.g. response.uploadId or response.correlationId) returned by the server and store that id on the client-side upload entries, then in setUploadingFiles find the entry by that uploadId instead of size and call setFilePreviews with the stable key (e.g. setFilePreviews(p => ({ ...p, [response.uploadId]: entry.previewUrl }))). Update any code that creates upload entries to include the client upload id so matching is deterministic.
🧹 Nitpick comments (2)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js (2)
350-358: ⚡ Quick winPreserve and restore original
URLmethods instead of deleting globals.At Line 356–Line 357, deleting global methods can pollute other tests. Store originals and restore them in
afterEachfor isolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js` around lines 350 - 358, The tests are deleting global URL methods which can leak into other tests; modify the beforeEach/afterEach so beforeEach saves the originals (e.g., const originalCreateObjectURL = URL.createObjectURL; const originalRevokeObjectURL = URL.revokeObjectURL) then mocks with jest.fn in beforeEach, and in afterEach restore them (URL.createObjectURL = originalCreateObjectURL; URL.revokeObjectURL = originalRevokeObjectURL) instead of using delete; update the beforeEach/afterEach in upload-input-v3.test.js around the existing beforeEach/afterEach blocks referencing URL.createObjectURL and URL.revokeObjectURL.
414-434: ⚡ Quick winAdd a collision test for parallel uploads with identical file sizes.
Current coverage validates out-of-order responses but only with unique sizes. Add a case where two image uploads have the same
sizeto catch preview mis-assignment regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js` around lines 414 - 434, Add a new unit test alongside the existing "correctly maps previews for parallel uploads using response size" that reproduces a collision where two uploads have identical size: use the same pattern of calling dropzoneCallbacks.onAddedFile for two distinct filenames (e.g., 'sunset.jpg' and 'portrait.jpg') with identical size values, call dropzoneCallbacks.onFileCompleted for both, then simulate server responses in reverse order via dropzoneCallbacks.onUploadComplete with server filenames (e.g., '246_portrait_abc123.jpg' and '246_sunset_def456.jpg'), rerender UploadInputV3 with value containing those server filenames and sizes, and assert that screen.getByRole('img', { name: ... }) returns the expected blob src mapping to the original preview names (e.g., 'blob:portrait.jpg' and 'blob:sunset.jpg') to catch preview mis-assignment when sizes collide.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Line 70: Add a useEffect that revokes cached blob URLs on unmount and whenever
filePreviews changes: implement useEffect(() => { return () => {
Object.values(filePreviews).forEach(url => { if (url) URL.revokeObjectURL(url)
}) } }, [filePreviews]); this ensures any object URLs created earlier (look for
places that call URL.createObjectURL in the upload preview code around the
filePreviews setter) are revoked; also ensure any code path that replaces an
individual preview calls URL.revokeObjectURL on the old URL before overwriting
filePreviews via setFilePreviews.
---
Duplicate comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 210-214: The current preview-assignment logic in the
setUploadingFiles callback uses response.size to match upload entries
(response.name/response.size and entry.previewUrl), which causes wrong previews
for concurrent same-size files; change the protocol to use a stable per-file
identifier (e.g. response.uploadId or response.correlationId) returned by the
server and store that id on the client-side upload entries, then in
setUploadingFiles find the entry by that uploadId instead of size and call
setFilePreviews with the stable key (e.g. setFilePreviews(p => ({ ...p,
[response.uploadId]: entry.previewUrl }))). Update any code that creates upload
entries to include the client upload id so matching is deterministic.
---
Nitpick comments:
In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js`:
- Around line 350-358: The tests are deleting global URL methods which can leak
into other tests; modify the beforeEach/afterEach so beforeEach saves the
originals (e.g., const originalCreateObjectURL = URL.createObjectURL; const
originalRevokeObjectURL = URL.revokeObjectURL) then mocks with jest.fn in
beforeEach, and in afterEach restore them (URL.createObjectURL =
originalCreateObjectURL; URL.revokeObjectURL = originalRevokeObjectURL) instead
of using delete; update the beforeEach/afterEach in upload-input-v3.test.js
around the existing beforeEach/afterEach blocks referencing URL.createObjectURL
and URL.revokeObjectURL.
- Around line 414-434: Add a new unit test alongside the existing "correctly
maps previews for parallel uploads using response size" that reproduces a
collision where two uploads have identical size: use the same pattern of calling
dropzoneCallbacks.onAddedFile for two distinct filenames (e.g., 'sunset.jpg' and
'portrait.jpg') with identical size values, call
dropzoneCallbacks.onFileCompleted for both, then simulate server responses in
reverse order via dropzoneCallbacks.onUploadComplete with server filenames
(e.g., '246_portrait_abc123.jpg' and '246_sunset_def456.jpg'), rerender
UploadInputV3 with value containing those server filenames and sizes, and assert
that screen.getByRole('img', { name: ... }) returns the expected blob src
mapping to the original preview names (e.g., 'blob:portrait.jpg' and
'blob:sunset.jpg') to catch preview mis-assignment when sizes collide.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 060f9ed7-3bec-49bb-a60b-9791fd9adcfc
📒 Files selected for processing (4)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/__tests__/progressive-img.test.jssrc/components/progressive-img/index.js
✅ Files skipped from review due to trivial changes (1)
- src/components/progressive-img/tests/progressive-img.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/progressive-img/index.js
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
LGTM, thank you @priscila-moneo
|
Memory leak: blob URLs not revoked on unmount
In a long-running SPA session where the user uploads images across multiple visits to this component without a full page reload, each unmount leaks the memory for all previewed images. Suggested fix — add a cleanup effect using a ref to capture latest state at unmount time: const filePreviewsRef = useRef(filePreviews);
filePreviewsRef.current = filePreviews;
const uploadingFilesRef = useRef(uploadingFiles);
uploadingFilesRef.current = uploadingFiles;
useEffect(() => {
return () => {
Object.values(filePreviewsRef.current).forEach(url => URL.revokeObjectURL(url));
uploadingFilesRef.current.forEach(f => f.previewUrl && URL.revokeObjectURL(f.previewUrl));
};
}, []);(Using refs instead of deps on |
|
Zero/undefined Two issues compound each other when a file's 1. const formatFileSize = (bytes) => {
if (!bytes) return '0 KB'; // catches 0, undefined, null, NaN
...
};A file whose 2. The size-based preview matching in const entry = prev.find(f => f.size === response.size && f.previewUrl);If Why these two are connected: the Suggested fix: guard the preview mapping against falsy sizes before attempting the match: const entry = response?.size
? prev.find(f => f.size === response.size && f.previewUrl)
: null;This ensures that a missing or zero size from the server never triggers a spurious preview assignment. |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please re review
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/upload-input-v3-image-preview-improvement
branch
from
ca65214 to
ee35ca1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 182-185: The upload cleanup in upload-input-v3 is removing every
completed row whenever value changes, which can hide other finished uploads that
the parent has not received yet. Update wrappedOnUploadComplete and the
useLayoutEffect cleanup to track each row’s upload/response identity, then only
clear the specific uploading row whose completed response is actually present in
value. Use the existing uploading row state in upload-input-v3 and the
completion handler logic around wrappedOnUploadComplete to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fd40445-f0a8-473b-a7f9-862a57f894c4
📒 Files selected for processing (4)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/__tests__/progressive-img.test.jssrc/components/progressive-img/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/progressive-img/tests/progressive-img.test.js
- src/components/inputs/upload-input-v3/tests/upload-input-v3.test.js
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/upload-input-v3-image-preview-improvement
branch
from
860645a to
0061872
Compare
…dComplete updater Move setFilePreviews call out of the setUploadingFiles updater function into useLayoutEffect, where it runs as a top-level side effect after the parent confirms the upload via the value prop. The updater is now pure: it computes matchedEntry from prev and tags the matched entry with serverFilename. useLayoutEffect then reads the current uploading entries via uploadingFilesRef, transfers any confirmed blob previews to filePreviews, and removes the completed rows — all batched into one render. This avoids a React contract violation (setState inside an updater is a side effect) and makes the code safe for React 18 Strict Mode, which double-invokes updater functions in development.
…DeleteUploading handleFileRemoved never called URL.revokeObjectURL, leaving blob URLs allocated when Dropzone's removedfile event fires through paths other than the custom delete button (e.g. programmatic removeFile calls, built-in Dropzone remove link). handleDeleteUploading had revokeObjectURL inside a setState updater (side effect in a pure function). Since delete always follows a committed render, the ref holds current state and the revocation can safely move outside the updater. handleFileError is intentionally left reading from prev inside the updater: Dropzone can fire error in the same synchronous tick as addedfile, before React commits the pending state update, so uploadingFilesRef is stale at that point. revokeObjectURL is idempotent so the side effect is safe there.
…of same-size images When two images with identical file sizes were uploaded in parallel, both onUploadComplete callbacks matched the same first uploadingFiles entry via prev.find(f => f.size === response.size && f.previewUrl), causing both server-renamed files to display the first image's blob preview. Fix: add !f.serverFilename to the find condition. Once an entry is matched and tagged with serverFilename, it is excluded from subsequent matches. This works because functional updaters always receive the accumulated queue state, so the second updater sees the first entry already claimed. Regression test: parallel upload of two images with identical size (10000 bytes), server responds in order — each server file must map to its own blob preview.
…vocation Covers the path where Dropzone fires removedfile directly (e.g. built-in remove link, programmatic removeFile) without going through the custom delete button. Previously untested — handleDeleteUploading was covered but handleFileRemoved was not.
…n parallel 202 uploads wrappedOnUploadComplete now uses dual size-based matching with a !serverFilename guard to tag only the specific entry that matches the server response, not all entries at the same size. This prevents a parallel HTTP-202 upload from prematurely marking a still-polling sibling file as complete. Adds regression test: two same-size files through 202 async path - only the file whose polling finishes gets marked complete.
…e file matching When the server returns original_name in the upload response, match the uploading entry by client filename instead of file size. This resolves the same-size parallel upload ambiguity definitively: two images of identical byte size now get correct previews even when server responses arrive out of order. The size-based dual matching is kept as a fallback for any consumer that has not yet deployed the updated file-upload-api.
3 participants
ref: https://app.clickup.com/t/86ba8wmgh
Summary by CodeRabbit
New Features
Improvements
data:/blob:sources.Tests